Use the correct language ID for JavaScript & TypeScript #1466
Conversation
hovsater
force-pushed
the
lsp-typescript
branch
2 times, most recently
from
901cc63 to
fe630fe
Compare
hovsater
changed the title
hovsater
changed the title
hovsater
changed the title
hovsater
force-pushed
the
lsp-typescript
branch
2 times, most recently
from
6962b63 to
01782ee
Compare
languages.toml
Outdated
| @@ -144,7 +144,8 @@ language-server = { command = "typescript-language-server", args = ["--stdio"] } | |||
| indent = { tab-width = 2, unit = " " } | |||
|
|
|||
| [[language]] | |||
| name = "tsx" | |||
| name = "typescriptreact" | |||
There was a problem hiding this comment.
I don't quite like this since now our language names are tied to what the LSP expects (which might also be different per language server). Let's instead add a language-id key to https://github.com/helix-editor/helix/blob/01782ee16adcffd56af44890ad772477dbefe610/helix-core/src/syntax.rs#L93-98
pub language_id: Option<String>Then the lsp code can use config.language_server.language_id.unwrap_or(config.name)
There was a problem hiding this comment.
Yeah, I thought so as well (I briefly mentioned it in the pull request description). I'll update the PR. 🙂
There was a problem hiding this comment.
Hmm, should it really be part of the LSP settings and not the language itself? It's not really a configuration of the LSP, but more specifically an identifier of the language itself. I think it would make more sense to put language-id under the language section. Then language-id could default to the language name if language-id wasn't present.
Thoughts?
There was a problem hiding this comment.
It's a lsp-specific language identifier. There's no standard conventions here, so I'd prefer to keep it grouped under the language server. It's quite likely that there could be another Javascript language server that expects a different identifier
hovsater
force-pushed
the
lsp-typescript
branch
from
01782ee to
c42377c
Compare
helix-view/src/document.rs
Outdated
| let fallback = self.language().and_then(|s| s.split('.').last()); | ||
|
|
||
| self.language | ||
| .as_ref() | ||
| .and_then(|config| config.language_server.as_ref()) | ||
| .and_then(|lsp_config| lsp_config.language_id.as_ref()) | ||
| .map_or(fallback, |id| Some(id.as_str())) |
There was a problem hiding this comment.
Let's use map.or_else(|| ...) to avoid computing the fallback unless it's needed. Also rsplit_once is a nice optimization over split(..).last():
| let fallback = self.language().and_then(|s| s.split('.').last()); | |
| self.language | |
| .as_ref() | |
| .and_then(|config| config.language_server.as_ref()) | |
| .and_then(|lsp_config| lsp_config.language_id.as_ref()) | |
| .map_or(fallback, |id| Some(id.as_str())) | |
| self.language | |
| .as_ref() | |
| .and_then(|config| config.language_server.as_ref()) | |
| .and_then(|lsp_config| lsp_config.language_id.as_ref()) | |
| .map_or_else( | |
| || self.language().and_then(|s| s.rsplit_once('.').map(|(_, last)| last)), | |
| |id| Some(id.as_str()) | |
| ) |
I'm also wondering why not
| let fallback = self.language().and_then(|s| s.split('.').last()); | |
| self.language | |
| .as_ref() | |
| .and_then(|config| config.language_server.as_ref()) | |
| .and_then(|lsp_config| lsp_config.language_id.as_ref()) | |
| .map_or(fallback, |id| Some(id.as_str())) | |
| self.language | |
| .as_ref() | |
| .and_then(|config| config.language_server.as_ref()) | |
| .map(|lsp_config| lsp_config.language_id.as_str()) | |
| .unwrap_or_else(|| self.language().and_then(|s| s.rsplit_once('.').map(|(_, last)| last))) |
There was a problem hiding this comment.
Fixed in cc80d3e.
I'm also wondering why not
I wasn't allowed to call as_str() on std::option::Option. On a more general note, I'm still very new to Rust, so my code is probably less than ideal most of the time. Happy to learn from feedback though. 🙂
helix-view/src/document.rs
Outdated
| /// Language ID for the document. Either the `language-id` from the | ||
| // `language-server` configuration, or the document language if no | ||
| /// `language-id` has been specified. |
There was a problem hiding this comment.
Missing / on second line?
hovsater
force-pushed
the
lsp-typescript
branch
from
48af651 to
cc80d3e
Compare
|
Looks good now, thanks :) 🎉 |
The TypeScript language server expects the
languageIdto bejavascriptfor.js,javascriptreactfor.jsx,typescriptfor.ts, andtypescriptreactfor.tsx. In practice, onlyjavascript,typescriptandtypescriptreactare relevant.Since the goal is to keep language scopes inline with what other editors are doing, I didn't want to change the scope for
tsxtosource.typescriptreact.Instead, I changed the implementation so that the language name is used as the
languageIdfor LSP. This works, since we can settree-sitter-libraryexplicitly if the language name doesn't match the name of the tree sitter library.While this doesn't seem to break any of the existing languages, I'm not sure if this is fine, or if we should introduce a new configuration option for languages called
language-idand only fallback to using the language name if it's not set.Any feedback is much appreciated. 🙂